[client] feat: Add support for displaying device code (UserCode) on Android TV…#4800
Conversation
WalkthroughAuthentication APIs were changed: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Auth
participant OAuthFlow
participant URLOpener
rect rgb(230,245,255)
Client->>Auth: Login(resultListener, urlOpener, isAndroidTV)
Auth->>OAuthFlow: NewOAuthFlow(ctx, cfg, isUnixDesktop, forceDeviceCodeFlow, hint)
end
rect rgb(240,255,230)
OAuthFlow->>Auth: flowInfo (VerificationURIComplete, UserCode)
Auth->>URLOpener: Open(flowInfo.VerificationURIComplete, flowInfo.UserCode)
Note right of URLOpener: opener receives URL + user code
end
rect rgb(255,240,240)
OAuthFlow->>Auth: Poll token / return tokenInfo
Auth->>Client: deliver token or error via resultListener
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5747004 to
748dce3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/internal/auth/oauth.go (1)
63-86: forceDeviceCodeFlow behavior is correct but the API is getting hard to readThe new
forceDeviceCodeFlowflag is wired correctly: it cleanly short-circuits to device code before OS heuristics and keeps existing PKCE → device-code fallback intact.However,
NewOAuthFlownow has two adjacentboolparameters, which is easy to misuse at call sites (isUnixDesktopClient, forceDeviceCodeFlow). Consider moving to an options struct or functional options if you expect further flags here, so callers become self-documenting and less error‑prone.client/android/login.go (3)
32-37: URLOpener comment should mention the new userCode argumentThe
URLOpenerinterface now exposes bothurlanduserCode, but the comment still only talks about showing a URL. Consider updating the comment to reflect that the callback may also be used to display the device/user code (especially for Android TV).
150-200: Android login: isAndroidTV wiring is correct; consider improving error propagationThreading
isAndroidTVfromAuth.Loginintologinis clean and non‑intrusive; for non‑TV Android the flow is unchanged, and for Android TV the flag is only used later to steer OAuth.While you’re touching this function, note that the backoff block:
err = a.withBackOff(a.ctx, func() error { err := internal.Login(a.ctx, a.config, "", jwtToken) if err == nil { go urlOpener.OnLoginSuccess() } if s, ok := gstatus.FromError(err); ok && (s.Code() == codes.InvalidArgument || s.Code() == codes.PermissionDenied) { return nil } return err })swallows
InvalidArgument/PermissionDeniedby returningnilto the backoff, sologincompletes without surfacing these errors toErrListener.OnError. If the intent is to stop retrying and report the failure, you could treat them as permanent errors instead:- if s, ok := gstatus.FromError(err); ok && (s.Code() == codes.InvalidArgument || s.Code() == codes.PermissionDenied) { - return nil - } - return err + if s, ok := gstatus.FromError(err); ok && (s.Code() == codes.InvalidArgument || s.Code() == codes.PermissionDenied) { + // stop retrying but still propagate the error to the caller + return backoff.Permanent(err) + } + return errThis would let
Auth.LogincallresultListener.OnError(...)when the server rejects the login, instead of treating it as a silent success of the retry loop.
202-224: Android foregroundGetTokenInfo: device‑code forcing and userCode exposure look good; minor context tweak possibleUsing
auth.NewOAuthFlow(a.ctx, a.config, false, isAndroidTV, "")is the right place to honorisAndroidTV: for TV you now force the Device Code flow; for phones/tablets (isAndroidTV == false) behavior stays as before.Passing both
VerificationURICompleteandUserCodeintourlOpener.Openenables the UI to render code‑entry instructions, which is exactly what Android TV needs.One small refinement you might consider for cancellation consistency is to avoid
context.TODO()here and reusea.ctx:- flowInfo, err := oAuthFlow.RequestAuthInfo(context.TODO()) + flowInfo, err := oAuthFlow.RequestAuthInfo(a.ctx)since you already derive the timeout for
WaitTokenfroma.ctx. Not required for correctness, but it would make the function’s context handling more coherent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/android/client.go(2 hunks)client/android/login.go(6 hunks)client/cmd/login.go(1 hunks)client/internal/auth/oauth.go(1 hunks)client/ios/NetBirdSDK/client.go(1 hunks)client/server/server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
client/ios/NetBirdSDK/client.go (1)
client/internal/auth/oauth.go (1)
NewOAuthFlow(70-86)
client/android/client.go (4)
client/android/login.go (1)
URLOpener(34-37)client/ios/NetBirdSDK/login.go (1)
URLOpener(33-35)client/android/dns_list.go (1)
DNSList(11-13)client/android/env_list.go (1)
EnvList(11-13)
client/server/server.go (1)
client/internal/auth/oauth.go (1)
NewOAuthFlow(70-86)
client/android/login.go (2)
client/ios/NetBirdSDK/login.go (3)
Auth(38-42)ErrListener(26-29)URLOpener(33-35)client/internal/auth/oauth.go (1)
NewOAuthFlow(70-86)
client/cmd/login.go (1)
client/internal/auth/oauth.go (1)
NewOAuthFlow(70-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: JS / Lint
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
🔇 Additional comments (5)
client/ios/NetBirdSDK/client.go (1)
215-256: NewOAuthFlow call updated correctly for new signaturePassing
falseforforceDeviceCodeFlowpreserves the previous PKCE-first behavior on iOS, with device code as fallback; no behavioral regression here.client/android/client.go (1)
94-127: Android Client.Run boolean plumbing looks consistentAdding
isAndroidTVtoRunand forwarding it toauth.login(urlOpener, isAndroidTV)cleanly threads the new flag without altering other logic; nothing stands out as problematic here.client/cmd/login.go (1)
325-357: CLI OAuth flow updated to new constructor without changing behaviorUsing
auth.NewOAuthFlow(ctx, config, isUnixRunningDesktop(), false, hint)keeps the existing desktop-vs-device-code behavior and simply adapts to the new parameter list.client/server/server.go (2)
500-550: Daemon Login SSO flow remains behaviorally unchanged
auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint)correctly adapts to the new signature while preserving the previous desktop detection and SSO flow selection logic.
1233-1261: JWT auth flow wiring matches updated NewOAuthFlow signatureIn
RequestJWTAuth, passingisDesktopandfalseintoauth.NewOAuthFlowkeeps SSH JWT authentication using the same PKCE/device-code selection logic as before; only the new parameter is being surfaced.
… SSO flow - Modified URLOpener interface to pass userCode alongside URL in login.go - added ability to force device auth flow
748dce3 to
c1bcec7
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/internal/auth/oauth.go (1)
63-86: Device‑flow selection logic looks good; update NewOAuthFlow comment to match behaviorThe extracted
shouldUseDeviceFlowhelper correctly handles:
- Forced device‑code flow (
forceDeviceCodeFlow == true), and- Headless Linux/FreeBSD (
!isUnixDesktopClienton those GOOS values).However, the
NewOAuthFlowcomment still claims it always “starts by initializing the PKCE” even though in these cases it goes straight to Device Code Flow. Consider tightening the comment so it reflects the conditional behavior, for example:-// It starts by initializing the PKCE.If this process fails, it resorts to the Device Code Flow, -// and if that also fails, the authentication process is deemed unsuccessful -// -// On Linux distros without desktop environment support, it only tries to initialize the Device Code Flow -// forceDeviceCodeFlow can be used to skip PKCE and go directly to Device Code Flow (e.g., for Android TV) +// By default it starts with the PKCE flow and falls back to Device Code Flow on failure. +// On Linux/FreeBSD without a desktop environment, or when forceDeviceCodeFlow is true, +// it uses the Device Code Flow directly. If both flows fail, authentication is deemed unsuccessful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/android/client.go(2 hunks)client/android/login.go(6 hunks)client/cmd/login.go(1 hunks)client/internal/auth/oauth.go(1 hunks)client/ios/NetBirdSDK/client.go(1 hunks)client/server/server.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/android/client.go
- client/android/login.go
🧰 Additional context used
🧬 Code graph analysis (3)
client/server/server.go (1)
client/internal/auth/oauth.go (1)
NewOAuthFlow(74-86)
client/cmd/login.go (1)
client/internal/auth/oauth.go (1)
NewOAuthFlow(74-86)
client/ios/NetBirdSDK/client.go (1)
client/internal/auth/oauth.go (1)
NewOAuthFlow(74-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: JS / Lint
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (4)
client/ios/NetBirdSDK/client.go (1)
231-255: NewOAuthFlow signature update for iOS mobile looks correctPassing
falseforisUnixDesktopClientandforceDeviceCodeFlowkeeps the existing PKCE‑first behavior with device‑code fallback and aligns with the new function signature.client/cmd/login.go (1)
335-356: CLI OAuth flow wiring preserved with new forceDeviceCodeFlow flagUsing
isUnixRunningDesktop()forisUnixDesktopClientandfalseforforceDeviceCodeFlowmaintains the prior PKCE vs device‑code behavior while matching the expanded NewOAuthFlow signature.client/server/server.go (2)
503-550: Login daemon call into NewOAuthFlow correctly threads desktop flagUsing
msg.IsUnixDesktopClientplusfalseforforceDeviceCodeFlowensures the daemon mirrors the client’s desktop/headless classification while keeping behavior identical for non‑Android‑TV callers.
1233-1261: SSH JWT auth flow correctly integrated with extended NewOAuthFlow
RequestJWTAuthnow callsNewOAuthFlow(ctx, config, isDesktop, false, hint), which preserves the existing PKCE/device‑code decision logic and simply accommodates the new parameter.



… SSO flow
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.